Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix solo machine proof height sequence mismatch in connection handshake verification #570

Merged
merged 3 commits into from
May 11, 2021

Conversation

colin-axner
Copy link
Contributor

ref: #562

Addresses one problem outlined in the issue above. Enforces that the proof height is equivalent to the current client state sequence.
NOTE: VerifyClientState is missing from ICS 03-connection as noted in #525. It is assumed that it will be inserted after VerifyConnection and before VerifyClientConsensusState

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should explain the height + 2 bit

Comment on lines +210 to +213
// ICS 003 will not increment the proof height after connection or client state verification
// the solo machine client must increment the proof height by 2 to ensure it matches
// the expected sequence used in the signature
abortTransactionUnless(height + 2 == clientState.consensusState.sequence)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is verify client consensus state always going to be called after 2 previous signature checks? It seems weird to hard code this delta in the verification function itself.

Copy link
Contributor Author

@colin-axner colin-axner May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you see the discussion in #562? I agree it is strange to hard code the delta in the verify function and that we cannot guarantee client consensus state verification will always occur in this order , but I don't see a more elegant solution. The proof height incrementing for each verification call is unique to the solo machine and thus cannot be brought into core IBC. Non-unique sequences is misbehaviour for the solo machine. Proof height may be meaningful to core IBC now or later and thus needs to be verified

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@cwgoes cwgoes merged commit d4e7af6 into master May 11, 2021
@cwgoes cwgoes deleted the colin/562-sm-sequence-fixes branch May 11, 2021 21:01
@cwgoes
Copy link
Contributor

cwgoes commented May 11, 2021

(merging with requisite 2/3 quorum from IBC spec committee)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants